[FLINK-13025] Elasticsearch 7.x support #9720

yanghua · 2019-09-19T12:12:00Z

What is the purpose of the change

This pull request implemented a new elasticsearch connector to support elasticsearch 7.x

Brief change log

Implemented a new connector to support elasticsearch 7.x client
Make old test support elasticsearch 7.x client

Verifying this change

This change is already covered by existing tests, such as ElasticsearchSinkITCase and Elasticsearch7UpsertTableSinkFactoryTest.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2019-09-19T12:15:01Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 67f07a9 (Wed Oct 16 08:43:21 UTC 2019)

Warnings:

6 pom.xml files were touched: Check for build and licensing issues.

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

yanghua · 2019-09-19T12:15:16Z

Note: This PR may not be ready for review especially for document support.

flinkbot · 2019-09-19T12:35:28Z

CI report:

d9c1dd5 : FAILURE Build
603db69 : SUCCESS Build
9bc5949 : FAILURE Build
53bfd62 : FAILURE Build
566cbff : SUCCESS Build
bbae033 : SUCCESS Build
2fbb118 : FAILURE Build
a89047b : FAILURE Build
5e0a6ff : FAILURE Build
bfeeca4 : FAILURE Build
3ba6a27 : FAILURE Build
430a4c1 : SUCCESS Build
555960a : SUCCESS Build
67f07a9 : SUCCESS Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

yanghua · 2019-09-20T10:08:36Z

@aljoscha What do you think about this PR? Anything that I missed, please let me know. Thanks

aljoscha · 2019-09-23T10:24:38Z

I had a cursory glance at this. The code seems fine, because it's basically the ES 6 code. I have some general comments:

I don't think the ES connectors (both 6 and 7) need the Table Planner dependency anymore. Maybe @twalthr can confirm this, but you can also try removing them and seeing what happens.
I don't think we need Table-specific code in the connectors when there is the flink-sql-connector-elasticsearch7 module. Maybe the Table-related code can be moved to that module, along with the tests.

yanghua · 2019-09-24T01:23:28Z

@aljoscha It seems you are right, we do not need the dependency of table planner.

Next, I will try to move the table-specific code from the es connector to SQL connector.

yanghua · 2019-09-25T09:46:19Z

@aljoscha I have addressed your comments (except moving table-specified code of flink-connector-elasticsearch6 to flink-sql-connector-elasticsearch6 module). Can you have another look?

aljoscha · 2019-09-26T10:07:42Z

Yes, I will definitely look at this!

aljoscha · 2019-09-27T07:03:43Z

flink-connectors/flink-sql-connector-elasticsearch7/src/main/resources/META-INF/NOTICE

@@ -0,0 +1,45 @@
+flink-sql-connector-elasticsearch7


why does this have a NOTICE file while the ES 6 connector doesn't?

Hi @aljoscha This NOTICE file referenced flink-sql-connector-elasticsearch6. The flink-connector-elasticsearch7 referenced flink-connector-elasticsrearch6, so they both have no NOTICE file.

Did you check whether the contents of the new NOTICE file is correct? A quick mvn dependency:tree on the new sql connector shows some dependencies that are not in the NOTICE file. I think these are new compared to ES6, which didn't have them. You can compare the output of mvn depdendency:tree on the ES6 sql jar and on the new ES7 sql jar and also compare to the NOTICE files.

Sorry, I forgot to check the dependency list. I will do the comparation soon.

@aljoscha I have run mvn depdendency:tree command and checked the license of every dependency through maven repository. Can you review again?

@twalthr Any other additional remarks?

Hi @aljoscha It seems @twalthr is busy recently? Can you talk with him offline to see if there is anything need to be considered?

Hi, this is not correct, the list of flink-shaded dependencies has nothing to do with the local shading in this connector. What I did now is to look at the output of mvn clean install in the ES7 connector directory. It has a section about what is included and what is not included. Based on this information I added some additional exclusions and I updated the NOTICE file: aljoscha@b80c8b9. I will apply this fix and then merge the PR.

If you are more attentive to these subtle problems, like dependencies and the NOTICE file, the PR review process could be a lot smoother.

@aljoscha Thanks. Actually, I am really not familar with NOTICE file and things about license.

Yes, it is easy to underestimate this. That is the problem, also for me. 😅

…and been shaded and excluded in maven-shade-plugin

aljoscha · 2019-10-14T15:48:26Z

I merged this. 👌

yanghua · 2019-10-15T01:17:27Z

@aljoscha Thanks for helping me to review this PR and finish this work.

yanghua · 2019-10-15T01:32:21Z

I had a cursory glance at this. The code seems fine, because it's basically the ES 6 code. I have some general comments:

I don't think we need Table-specific code in the connectors when there is the flink-sql-connector-elasticsearch7 module. Maybe the Table-related code can be moved to that module, along with the tests.

@aljoscha When reviewed your first comment, it seems this is also a issue for ES 6 connector and sql connector. WDYT? Shall we fixed it?

aljoscha · 2019-10-15T07:35:33Z

I think we can do that, yes.

yanghua · 2019-10-15T07:46:38Z

OK, I have created a Jira issue to track this problem. Please see: FLINK-14395

li-zhao-hotstar · 2019-12-12T07:58:26Z

Hi @yanghua
Thanks for the effort.
Came from https://issues.apache.org/jira/browse/FLINK-13025, want to check is this available now?
https://ci.apache.org/projects/flink/flink-docs-stable/dev/connectors/elasticsearch.html still didn't reflect the update, also can not find flink-connector-elasticsearch7_2.11 in any repo.

rmetzger added the review=description? label Sep 19, 2019

rmetzger added the component=Connectors/ElasticSearch label Sep 19, 2019

yanghua force-pushed the FLINK-13025 branch 2 times, most recently from 603db69 to 9bc5949 Compare September 20, 2019 03:14

[FLINK-13025] Elasticsearch 7.x support

53bfd62

yanghua force-pushed the FLINK-13025 branch from 9bc5949 to 53bfd62 Compare September 20, 2019 05:50

Removed duplicated dependency

566cbff

Removed table planner dependency from es 6 and 7 connector

bbae033

Moved table-specific code from ES7 connector to ES7 sql connector

a89047b

yanghua force-pushed the FLINK-13025 branch from 2fbb118 to a89047b Compare September 24, 2019 06:30

Removed deprecated type field

e685d22

yanghua force-pushed the FLINK-13025 branch 2 times, most recently from bfeeca4 to 3ba6a27 Compare September 25, 2019 02:36

Removed type field for elasticsearch table sink

430a4c1

yanghua force-pushed the FLINK-13025 branch from 3ba6a27 to 430a4c1 Compare September 25, 2019 06:47

aljoscha reviewed Sep 27, 2019

View reviewed changes

yanghua added 2 commits September 29, 2019 15:29

Updated NOTICE license file

555960a

Removed some dependencies which are test and transitive dependencies …

67f07a9

…and been shaded and excluded in maven-shade-plugin

aljoscha closed this Oct 14, 2019

yanghua mentioned this pull request Oct 16, 2019

[FLINK-14395] Refactor ES 6 connector to split table-specific code into flink-sql-connector-elasticsearch6 #9906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-13025] Elasticsearch 7.x support #9720

[FLINK-13025] Elasticsearch 7.x support #9720

yanghua commented Sep 19, 2019

flinkbot commented Sep 19, 2019 •

edited

yanghua commented Sep 19, 2019

flinkbot commented Sep 19, 2019 •

edited

yanghua commented Sep 20, 2019

aljoscha commented Sep 23, 2019

yanghua commented Sep 24, 2019

yanghua commented Sep 25, 2019

aljoscha commented Sep 26, 2019

aljoscha Sep 27, 2019

yanghua Sep 27, 2019

aljoscha Sep 27, 2019

yanghua Sep 27, 2019

yanghua Sep 29, 2019

yanghua Oct 8, 2019

yanghua Oct 14, 2019

aljoscha Oct 14, 2019

yanghua Oct 14, 2019

aljoscha Oct 14, 2019

aljoscha commented Oct 14, 2019

yanghua commented Oct 15, 2019

yanghua commented Oct 15, 2019

aljoscha commented Oct 15, 2019

yanghua commented Oct 15, 2019

li-zhao-hotstar commented Dec 12, 2019

[FLINK-13025] Elasticsearch 7.x support #9720

[FLINK-13025] Elasticsearch 7.x support #9720

Conversation

yanghua commented Sep 19, 2019

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Sep 19, 2019 • edited

Automated Checks

Review Progress

yanghua commented Sep 19, 2019

flinkbot commented Sep 19, 2019 • edited

CI report:

yanghua commented Sep 20, 2019

aljoscha commented Sep 23, 2019

yanghua commented Sep 24, 2019

yanghua commented Sep 25, 2019

aljoscha commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljoscha commented Oct 14, 2019

yanghua commented Oct 15, 2019

yanghua commented Oct 15, 2019

aljoscha commented Oct 15, 2019

yanghua commented Oct 15, 2019

li-zhao-hotstar commented Dec 12, 2019

flinkbot commented Sep 19, 2019 •

edited

flinkbot commented Sep 19, 2019 •

edited